AllowPrivilegeEscalation: add validations #52803

jessfraz · 2017-09-20T17:36:30Z

Fixes #52454

dims · 2017-09-20T17:38:20Z

/assign @lavalamp

dims · 2017-09-20T17:38:35Z

/release-note-none

jessfraz · 2017-09-20T17:48:44Z

sorry i messed something up 1 sec

…

-- Jessie Frazelle 4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3 pgp.mit.edu <http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3>

tallclair · 2017-09-20T17:53:37Z

pkg/api/v1/defaults.go

+
+	// handle the case for AllowPrivilegeEscalation when the container is privileged
+	if sc.Privileged != nil && *sc.Privileged {
+		sc.AllowPrivilegeEscalation = &pt


Hmm, I'm not sure it's acceptable for defaulting code to change values that have been set. Maybe it would be better for this to go in validation (i.e. setting AllowPrivilegeEscalataion == false && Privileged == true is an error)

updated in a differnt commit, can squash before merge, added tests too

tallclair · 2017-09-20T17:56:32Z

pkg/kubelet/kuberuntime/security_context.go

@@ -69,7 +69,8 @@ func (m *kubeGenericRuntimeManager) determineEffectiveSecurityContext(pod *v1.Po
 		synthesized.SupplementalGroups = append(synthesized.SupplementalGroups, groups...)
 	}

-	synthesized.NoNewPrivs = securitycontext.AddNoNewPrivileges(effectiveSc)
+	// Set the NoNewPrivs setting to the opposite of AllowPrivilegeEscalation.
+	synthesized.NoNewPrivs = !*effectiveSc.AllowPrivilegeEscalation


nit: handle the nil case. e.g. for static pods (just default to true?)

dims · 2017-09-20T20:06:32Z

@jessfraz are these legit failures? (let me try a retest in case they are flakes)

/retest

jessfraz · 2017-09-20T21:14:15Z

Do you know why TestRoundTripTypes could have broken, it looks like it's doing a fuzz so I'm unsure how the Defaults behavior might have messed with that?

update: figured this one out but the other mystery remains with the e2e-node tests

dims · 2017-09-20T23:46:12Z

ah fuzzers!

/test pull-kubernetes-node-e2e

dims · 2017-09-21T14:17:08Z

pkg/api/validation/validation.go

+	}
+
+	if sc.AllowPrivilegeEscalation != nil && !*sc.AllowPrivilegeEscalation {
+		if *sc.Privileged {


@jess, found the trace backs : http://paste.openstack.org/show/621629/

Looks like we need to check for nil here

if sc.Privileged != nil && *sc.Privileged

omg you are my new favorite person on the planet

dims · 2017-09-21T14:18:19Z

@jessfraz please see inline comment, found the panic traceback

php-coder · 2017-09-21T15:17:38Z

pkg/api/v1/defaults.go

@@ -389,3 +389,32 @@ func SetDefaults_HostPathVolumeSource(obj *v1.HostPathVolumeSource) {
 		obj.Type = &typeVol
 	}
 }
+
+func SetDefaults_SecurityContext(sc *v1.SecurityContext) {
+	pt := true


For consistency, it should have name ape (as in the fuzzer).

can do, I was debating over what naming I liked better, honestly I just wish &true and &false worked

php-coder · 2017-09-21T15:20:07Z

pkg/api/v1/defaults.go

+	if sc.Capabilities != nil {
+		for _, cap := range sc.Capabilities.Add {
+			if string(cap) == "CAP_SYS_ADMIN" {
+				sc.AllowPrivilegeEscalation = &pt


break is missing here.

php-coder · 2017-09-21T15:24:16Z

pkg/api/v1/defaults.go

+	}
+
+	// handle the case for AllowPrivilegeEscalation when we are adding CAP_SYS_ADMIN
+	if sc.Capabilities != nil {


This if can be replaced by else if to prevent inspecting capabilities when we've already set AllowPrivilegeEscalation to true in the previous branch.

php-coder · 2017-09-21T15:28:17Z

pkg/api/v1/defaults.go

+	}
+
+	// handle the case where the user did not set the defaultAllowPrivilegeEscalation and did not explicitly set allowPrivilegeEscalation
+	if sc.AllowPrivilegeEscalation == nil {


Comment assumes that defaultAllowPrivilegeEscalation == nil but code doesn't check for that. Is it intentionally?

yeah I was just trying to relay that the securityContext would be nil if they didn't set a default.

php-coder · 2017-09-21T15:30:46Z

pkg/api/v1/defaults_test.go

+		"allowPrivilegeEscalation nil security context nil": {
+			expect: true,
+		},
+		"allowPrivilegeEscalation nil capAddSysadmin": {


s/nil capAddSysadmin/and CAP_SYS_ADMIN/

Ah, I see the logic in the names now... Could you make it more explicit? Otherwise it could be misinterpreted which exactly value is nil in the cases like allowPrivilegeEscalation nil privileged

php-coder · 2017-09-21T15:39:04Z

pkg/api/validation/validation.go

+		if sc.Capabilities != nil {
+			for _, cap := range sc.Capabilities.Add {
+				if string(cap) == "CAP_SYS_ADMIN" {
+					allErrs = append(allErrs, field.Forbidden(field.NewPath(""), "Cannot set `allowPrivilegeEscalation` to false and `capAdd` CAP_SYS_ADMIN"))


There is no field capAdd. Let's use capabilities.add at least.

Plase, add break here to stop early.

tallclair · 2017-09-25T16:59:04Z

/lgtm
Thanks!

dchen1107 · 2017-09-25T17:10:59Z

pkg/api/validation/validation.go

+
+	if sc.AllowPrivilegeEscalation != nil && !*sc.AllowPrivilegeEscalation {
+		if sc.Privileged != nil && *sc.Privileged {
+			allErrs = append(allErrs, field.Invalid(fldPath, sc, "Cannot set `allowPrivilegeEscalation` to false and `privileged` to true"))


Error string start with lower case?

dchen1107 · 2017-09-25T17:11:06Z

pkg/api/validation/validation.go

+		if sc.Capabilities != nil {
+			for _, cap := range sc.Capabilities.Add {
+				if string(cap) == "CAP_SYS_ADMIN" {
+					allErrs = append(allErrs, field.Invalid(fldPath, sc, "Cannot set `allowPrivilegeEscalation` to false and `capabilities.Add` CAP_SYS_ADMIN"))


dchen1107 · 2017-09-25T17:11:52Z

Small nits, otherwise look good to me.

liggitt · 2017-09-25T17:16:33Z

this is release blocking... can we merge/pick this as-is for 1.8, and follow up with the message tweaks?

jessfraz · 2017-09-25T17:19:49Z

I can update the PR real fast no worries

Signed-off-by: Jess Frazelle <acidburn@microsoft.com>

liggitt · 2017-09-25T17:33:07Z

/lgtm
marking approved per #52803 (comment)

dchen1107 · 2017-09-25T17:43:47Z

/lgtm

k8s-github-robot · 2017-09-25T17:44:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, dims, jessfraz, liggitt, tallclair

Associated issue: 52454

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/OWNERS~~ [dchen1107]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

jessfraz · 2017-09-25T18:20:31Z

/test pull-kubernetes-e2e-gce-bazel

abgworrall · 2017-09-25T19:36:48Z

Changing cherrypick- labels, to account for the fact that this master PR has already been cherrypicked into a base:release-1.8 PR and sent on to the submit queue, and so doesn't need attention from the batch cherrypick tooling.

k8s-github-robot · 2017-09-25T21:50:53Z

Automatic merge from submit-queue (batch tested with PRs 51067, 52319, 52803, 52961, 51972). If you want to cherry-pick this change to another branch, please follow the instructions here..

Automatic merge from submit-queue. Cherry-pick #52803: AllowPrivilegeEscalation: add validations Cherry-pick of #52803: AllowPrivilegeEscalation: add validations Fixes #52454

@jessfraz

…n_test Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Move test cases to a proper test method **What this PR does / why we need it**: Prior this change, we had tests in `TestValidatePodSpec()` method that is designated for testing `ValidatePodSpec()`. But because we test code from the `ValidateSecurityContext()` method, the tests should belong to `TestValidateSecurityContext()`. This PR improves the tests. Now the tests become less fragile because `ValidatePodSpec()` do a lot more validations than `ValidateSecurityContext()`. **Which issue(s) this PR fixes**: Related to #52803 where this code and tests were introduced. **Release note**: ```release-note NONE ``` PTAL @jessfraz CC @simo5

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 20, 2017

jessfraz requested a review from tallclair September 20, 2017 17:36

jessfraz added this to the v1.8 milestone Sep 20, 2017

k8s-github-robot assigned timothysc and tallclair Sep 20, 2017

k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 20, 2017

k8s-ci-robot assigned lavalamp Sep 20, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 20, 2017

tallclair reviewed Sep 20, 2017

View reviewed changes

jessfraz force-pushed the AllowPrivilegeEscalation branch 2 times, most recently from 90d9903 to 0b4086e Compare September 20, 2017 18:37

jessfraz force-pushed the AllowPrivilegeEscalation branch from 0b4086e to 357eef9 Compare September 20, 2017 20:37

dims reviewed Sep 21, 2017

View reviewed changes

jessfraz force-pushed the AllowPrivilegeEscalation branch 2 times, most recently from 26506c0 to 2fa9bf5 Compare September 21, 2017 15:00

php-coder reviewed Sep 21, 2017

View reviewed changes

spiffxp mentioned this pull request Sep 25, 2017

AllowPrivilegeEscalation missing API validation #52454

Closed

dchen1107 reviewed Sep 25, 2017

View reviewed changes

AllowPrivilegeEscalation: add validations for caps and privileged

0ad51ed

Signed-off-by: Jess Frazelle <acidburn@microsoft.com>

jessfraz force-pushed the AllowPrivilegeEscalation branch from 0c610d5 to 0ad51ed Compare September 25, 2017 17:22

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 25, 2017

jessfraz mentioned this pull request Sep 25, 2017

Cherry-pick #52803: AllowPrivilegeEscalation: add validations #53003

Merged

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 25, 2017

liggitt added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 25, 2017

liggitt modified the milestones: v1.8, v1.9 Sep 25, 2017

abgworrall added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed cherrypick-candidate labels Sep 25, 2017

k8s-github-robot merged commit 3090fe5 into kubernetes:master Sep 25, 2017

jessfraz deleted the AllowPrivilegeEscalation branch September 25, 2017 21:51

php-coder mentioned this pull request Jan 31, 2018

Move test cases to a proper test method #59137

Merged

AllowPrivilegeEscalation: add validations #52803

AllowPrivilegeEscalation: add validations #52803

Conversation

jessfraz commented Sep 20, 2017

dims commented Sep 20, 2017

dims commented Sep 20, 2017

jessfraz commented Sep 20, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dims commented Sep 20, 2017

jessfraz commented Sep 20, 2017 • edited

dims commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dims commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallclair commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchen1107 commented Sep 25, 2017

liggitt commented Sep 25, 2017

jessfraz commented Sep 25, 2017

liggitt commented Sep 25, 2017

dchen1107 commented Sep 25, 2017

k8s-github-robot commented Sep 25, 2017

jessfraz commented Sep 25, 2017

abgworrall commented Sep 25, 2017

k8s-github-robot commented Sep 25, 2017

jessfraz commented Sep 20, 2017 •

edited